-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(lsp): Implement textDocument/rename #8910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. Just few comments.
@@ -1143,4 +1185,57 @@ mod tests { | |||
]); | |||
harness.run().await; | |||
} | |||
#[tokio::test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test 👍
cli/lsp/tsc.rs
Outdated
impl RenameLocations { | ||
pub async fn into_workspace_edit( | ||
self, | ||
ls: &LanguageServer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RenameLocations
can have the locations that is not same to current document so we should accept LanguageServer
to resolve line_index
per each locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to use the same approach as DefinitionInfoAndBoundSpan::to_definition
. The index_provider
argument there is probably what you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it with referencing DefinitionInfoAndBoundSpan::to_definition
.
It will never support it, so the resulting behaviour is the desired behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, very nice contribution... thank you!
Related #8643
Currently, the ConfigureRequest (in
cli/tsc/compiler.d.ts
) does not supporttsconfig.json
'sinclude/exclude
field.So the tsserver will rename symbols in only the opened text document or explicitly dependent text document.